fix(frontend): clean up @latest frontend downloads (CORE-285)#14398
fix(frontend): clean up @latest frontend downloads (CORE-285)#14398DrJKL wants to merge 5 commits into
Conversation
Old ComfyUI_frontend releases pulled in via --front-end-version <repo>@latest were never removed, causing web_custom_versions/ to grow unbounded (one user reported 5.11 GB). When the user requests an auto-resolving specifier (@latest or @prerelease), record which concrete version was materialized in a small .auto_managed.json next to the per-provider folders. On a subsequent auto-managed run, prune any previously-tracked folder that is no longer the current one. Explicitly pinned versions (e.g. @v1.46.0) are left alone, and if a user explicitly pins a version that had previously been downloaded under @latest, it is promoted out of the auto-managed set so it survives future cleanup.
Addresses review feedback on the auto-managed metadata helpers: - json.load() on the metadata file can return non-dict values (e.g. a bare list or a string); guard the root type before calling .get(). - A tampered or hand-edited .auto_managed.json could contain entries like '../escape'. The previous code happily fed those into rmtree. Filter such entries out at both read time and write time so they never reach disk or cleanup, and add a belt-and-suspenders path containment check in _prune_auto_managed_versions that requires the resolved target to live strictly under the resolved provider dir.
There was a problem hiding this comment.
Way too many long comments.
There was a problem hiding this comment.
Fair — trimmed in 0a2b9f8. Net -76 lines of prose across the two files:
- Killed all narrative/scenario comments inside test bodies and helper functions (they restated the test setup).
- Removed the multi-line block comments on
AUTO_MANAGED_VERSION_SPECIFIERSand the regex constant — kept tight one-liners. - Collapsed the
init_frontend_unsafe"promote/prune" comments into nothing — the method names already say it. - Shortened docstrings on
_read/_prune/_untrack_auto_managed_versionsto one line each.
Kept just five short comments that aren't restating code:
AUTO_MANAGED_VERSION_SPECIFIERSconstant (says why this tuple exists vs explicit pins)- The regex (security: explains the allowlist purpose)
- The atomic-write step (non-obvious tmp+rename pattern)
- The defense-in-depth
_is_safe_version_dirnamere-check in_prune_auto_managed_versions(would otherwise look redundant and get "simplified" away)
33/33 tests still pass.
There was a problem hiding this comment.
Cut further in 273314a — only one comment remains in the new code:
# Re-check even though read already filters: keeps rmtree provably
# bounded under provider_dir regardless of caller.
if not cls._is_safe_version_dirname(stale_version):
...This one I'd push back on removing: the _is_safe_version_dirname re-check inside _prune_auto_managed_versions looks redundant (the read path already filters), and without a comment a future maintainer will simplify it away. That would re-open the path-traversal vector Oracle flagged in the first review pass. The test test_prune_refuses_to_delete_outside_provider_dir would still pass without the re-check (because it relies on the resolve-and-check below it), so the comment is the only thing protecting the layered defense.
Everything else — class-constant explanations, regex purpose, atomic-write idiom, all docstrings on the new private helpers — is gone. Happy to delete this last one too if you'd rather rename the helper or merge the check into _is_safe_version_dirname's docstring instead.
33/33 tests still pass.
Replace the .auto_managed.json sidecar with a .auto_managed/ marker directory. Tracked version names now come from real single-component dirents, removing the untrusted-input parsing, path-traversal checks, and rmtree boundary guards that the JSON design required. Also fix the dead Optional handling in _prune_auto_managed_versions, extract _ensure_release_downloaded to drop the download_succeeded flag, and reuse _provider_dir in init_frontend_unsafe. Amp-Thread-ID: https://ampcode.com/threads/T-019eb879-1e6f-77aa-abb3-4d229d18061f Co-authored-by: Amp <amp@ampcode.com>
2 participants
PR Created by the Glary-Bot Agent
Summary
Fixes CORE-285: old
ComfyUI_frontendreleases pulled in via--front-end-version <repo>@latestaccumulate inweb_custom_versions/and never get cleaned up. One user reported 5.11 GB of stale downloads.Approach
Per Austin's suggestion in the thread: distinguish between auto-resolving specifiers (
@latest,@prerelease) and explicit pins (@v1.46.0,@1.46.0). Auto-cleanup applies only to the former. No new CLI flag — design matches the thread consensus.A small hidden metadata file
.auto_managed.jsonis written next to the per-provider release folders (e.g.web_custom_versions/Comfy-Org_ComfyUI_frontend/.auto_managed.json). It records which concrete versions were materialized via an auto-resolving specifier. Whenever the user re-runs with@latest/@prerelease, any tracked-but-no-longer-current folder isshutil.rmtree'd before the metadata is rewritten.Explicitly pinned versions are never touched. As a bonus, if a user later pins a version that had previously been downloaded under
@latest, it is promoted out of the auto-managed set so it survives the next cleanup pass — useful for the bisecting workflow mentioned in the thread.Changes
app/frontend_management.pyAUTO_MANAGED_VERSION_SPECIFIERS = ("latest", "prerelease")+ metadata filename constant._read_auto_managed_versions,_write_auto_managed_versions(atomic temp file +os.replace),_prune_auto_managed_versions,_untrack_auto_managed_version,_is_safe_version_dirname.init_frontend_unsafe, after a successful download or hit-on-disk:tests-unit/app_test/frontend_manager_test.py@v1.0.0and@1.0.0forms, and a path-containment defense-in-depth check.Robustness (post-review hardening)
Oracle review surfaced two issues — both addressed:
json.load()can return a non-dict root ([],null,"oops",42)..get(...)would have crashed. Now weisinstance(data, dict)check before reading and ignore anything else..auto_managed.jsoncould contain"../escape". Defense in depth:_is_safe_version_dirname(allowlist regex, rejects..,/,\, NUL, etc.) before being returned._prune_auto_managed_versionsresolves both the provider directory and each candidate path, then refuses tormtreeanything that isn't strictly under the resolved provider directory. Symlinks and path tricks can't escape.Verification
frontend_manager_test.py(18 pre-existing + 15 new). Pre-existing failures elsewhere intests-unit/are unrelated (user_manager, model_management, etc. — same count onmaster).init_frontend_unsafethrough a 5-step scenario (1st @latest → 2nd @latest → explicit pin → 3rd @latest → identical 4th @latest). Disk state after each step matched the design: stale auto-managed folders pruned, pinned folder preserved through subsequent cleanups.lsp_diagnostics): zero new errors introduced; pre-existing diagnostics unchanged.Backward compatibility / migration
web_custom_versions/is already cluttered won't have those existing folders retroactively cleaned (no metadata exists for them yet). The next@latestinvocation starts tracking from that point forward and cleans up on subsequent runs of new@latestversions. Old cruft can be manually deleted once. A retroactive sweep would require guessing which existing folders were@latestvs explicit pins, which we can't reconstruct.--front-end-versionflag, using the pip-shipped frontend) is unchanged.